Fixed zero insertion when concatenate sparse matrices with 0*I #21657
Conversation
|
LGTM, but is a behavior change so should merge post-branching |
What do you mean? |
|
@tkelman means that we shouldn't merge this change until after the release-0.6 branch is created because this is a user-visible behavior change (albeit not likely to be a terribly breaking one). |
|
Oh, cool. |
test/sparse/sparse.jl
Outdated
| @@ -47,12 +47,18 @@ end | |||
| @testset "concatenation tests" begin | |||
| @testset "horizontal concatenation" begin | |||
| @test all([se33 se33] == sparse([1, 2, 3, 1, 2, 3], [1, 2, 3, 4, 5, 6], ones(6))) | |||
|
|
|||
| sp33 = speye(3, 3) | |||
There was a problem hiding this comment.
This sp33 definition seems unnecessary given its absence in the immediately following test, and likewise two lines below? Or am I missing something? (Also, why the additional empty lines?)
test/sparse/sparse.jl
Outdated
| @testset "horizontal concatenation" begin | ||
| @test all([se33 se33] == sparse([1, 2, 3, 1, 2, 3], [1, 2, 3, 4, 5, 6], ones(6))) | ||
| @test all([se33 se33] == sparse([1, 2, 3, 1, 2, 3], [1, 2, 3, 4, 5, 6], ones(6))) |
|
Thanks @iamnapo! |
| @@ -1430,6 +1430,9 @@ speye_scaled(diag, m::Integer, n::Integer) = speye_scaled(typeof(diag), diag, m, | |||
|
|
|||
| function speye_scaled(T, diag, m::Integer, n::Integer) | |||
| ((m < 0) || (n < 0)) && throw(ArgumentError("invalid array dimensions")) | |||
| if iszero(diag) | |||
| return SparseMatrixCSC(m, n, ones(Int, n+1), Vector{Int}(0), Vector{T}(0)) | |||
There was a problem hiding this comment.
probably not very consequential, but I wonder how often this gets called in a way that makes the Int(m), Int(n) conversions below necessary
There was a problem hiding this comment.
I think none. I even think the ones below are unnecessary, now that you mention it.
There was a problem hiding this comment.
maybe if you called it directly with a non Int sized integer?
There was a problem hiding this comment.
Won't the constructor of SparseMatrixCSC take care of that?
There was a problem hiding this comment.
Yes, it looks like the SparseMatrixCSC constructor takes care of the Int conversions, so they could be removed here.
There was a problem hiding this comment.
(I think is easier if someone changes it directly, instead of me opening another PR just for that.)
Fixes #21563